Skip to content
This repository was archived by the owner on Sep 2, 2022. It is now read-only.

Improved code style and legibility and added limit option#2

Closed
eusonlito wants to merge 5 commits intojakerella:masterfrom
eusonlito:master
Closed

Improved code style and legibility and added limit option#2
eusonlito wants to merge 5 commits intojakerella:masterfrom
eusonlito:master

Conversation

@eusonlito
Copy link

I hope that can be interesting for you.

  • Some code improvements (nested ifs and preload regular expressions).
  • Improved styles removing some strong colors.
  • Added limit option.

@jakerella
Copy link
Owner

Thanks! I'm at a conference, but I'll try to review soon.

@eusonlito
Copy link
Author

Hi! remember me :)

@jakerella
Copy link
Owner

SO SORRY!
Totally my bad here. I've bookmarked this and will get to it in 2 weeks... you won't believe it, but I'm about to head out on vacation again. My first one since back then. ;)

@eusonlito
Copy link
Author

No problem!! I'm not your boss ;)

@jakerella
Copy link
Owner

Hello again... I want to thank you for working so hard on this code. I really hope that it has served you well. That said, you've submitted an enormous amount of changes for such a small library. In addition to the limit option there are numerous changes that aren't just "style" or "legibility" but large reorganizations of the code. I'm not opposed to large refactoring, but it's difficult for me to process such a large change, especially without unit tests (which is 100% my fault).

At this point, I don't think I can just merge this PR in, there's just too much that I can't verify easily. Of course, you can always keep your fork active, and I would be happy to merge in the limit functionality by itself if you wanted to submit that on its own. Additionally, if we were to create unit tests I'd be much more comfortable looking at these changes.

Essentially this just comes down to there being way too much for a single PR. Hope this doesn't deter you from helping this or other projects!!

@eusonlito
Copy link
Author

Ok, no problem! Anyway thanks for chek my code. Regards!

@eusonlito eusonlito closed this Aug 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants